fix(auth-server): handle cancelled billing agreement NVP error - #21092
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Handles cancelled PayPal billing agreements without failing Stripe invoice.created webhooks.
Changes:
- Maps PayPal NVP error 10201 to the existing internal validation error.
- Adds tests for translation and passthrough of other PayPal errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/fxa-auth-server/lib/payments/paypal/helper.ts |
Translates cancelled-agreement API errors. |
packages/fxa-auth-server/lib/payments/paypal/helper.spec.ts |
Covers translation and rethrow behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
xlisachan
reviewed
Aug 24, 2026
| ) { | ||
| throw error.internalValidationError('updateStripeNameFromBA', { | ||
| message: 'Billing agreement was cancelled.', | ||
| }); |
Contributor
There was a problem hiding this comment.
internalValidationError(op, data, error) takes a third error arg. We should preserves the PayPal error as the cause -
throw error.internalValidationError(
'updateStripeNameFromBA',
{ message: 'Billing agreement was cancelled.' },
err
);
xlisachan
approved these changes
Aug 24, 2026
xlisachan
left a comment
xlisachan
left a comment
Contributor
There was a problem hiding this comment.
r+wc, thanks Davey!
Just a nit regarding keeping the original err.
Because: - PayPal rejects the agreement lookup with NVP error 10201 instead of returning a cancelled status, so the guard added in 93f31f9 never runs. - The unrecognized error escapes handleInvoiceCreatedEvent as a 500, which Stripe retries and Sentry reports. This commit: - Translates error 10201 in updateStripeNameFromBA into the internal validation error the webhook already handles, keeping the PayPal error as the cause. - Covers the new branch and the rethrow path in helper.spec.ts. Closes PAY-3778
david1alvarez
force-pushed
the
PAY-3778
branch
from
August 25, 2026 00:32
0b2155f to
bfc7b79
Compare
david1alvarez
enabled auto-merge
August 25, 2026 00:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
invoice.createdwebhooks return 500, so Stripe retries the delivery and Sentry reports every attempt (FXA-AUTH-2TC).This pull request
updateStripeNameFromBAinto the internal validation error the webhook already handles.Issue that this pull request solves
Closes: PAY-3778
Checklist
Put an
xin the boxes that applyHow to review (Optional)
updateStripeNameFromBA.helper.ts, then the two new tests.stripe-webhook.tsmatching on errno, which this PR does not touch. That contract is now asserted in the helper spec.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
The stale billing agreement is deliberately left on the Stripe customer. The ticket does not ask for it, and the IPN
mpCancelpath already owns clearing agreements.No pre-merge review skill was run: the production change is 13 lines reusing an existing in-file idiom (
processor.ts:243) and mutates no state.